Conversation
pull request template 파일 수정
중복 파일 제거
pull request template 수정
a 태그 안 li 태그 -> li 태그 안 a 태그
- IncludeComponent.js 파일에 있었던 컴포넌트 로드 스크립트를 ComponentCheckFunction.js 파일로 분리 - 코드 재사용성 향상 - JS 파일 이름 수정
- ComponentLoader.js HtmlLoader.js 콜백함수 분리
- 버튼 disabled 속성 제거, input 값에 따른 버튼 속성 변경 관련 이벤트 리스너와 스크립트 제거
- 로고 앵커 태그 링크 수정 - 로고 가운데 정렬
- 미디어쿼리 삭제 및 rem을 px로 고침
- 글로벌을 제외한 페이지별 css 파일만 만들어둔 상태며 작업은 하지 않음
- Home.html의 태블릿, 모바일 반응형 구현 - 아무 내용 없는 css 파일 삭제
- 바텀 이미지 크기 수정
- 태블릿 모바일 반응형 구현 - 로그인 페이지 로고 이미지 수정 - 관련 html 파일 래퍼 영역 변경 ## 발생된 이슈 - 인풋 outline 적용안됨
- 태블릿 모바일 추가 구현
- 회원가입 데스크탑 기준 css 적용 에러 문제 해결 - 눈 아이콘을 나타내는 태그 변경 및 관련 css, js 파일 수정
- input 속성 required 부분을 삭제하고 focusin, focusout이벤트 기준으로 경고 메시지를 뜨게끔 수정 - 그에 따라 html 코드 밑에 p태그를 추가해 메시지를 나타낼 때 요소 너비를 차지하도록 추가 - 관련 JS 코드 처리 및 리팩토링
- bottom 기준 position 잡았던 것을 top 기준으로 변경 - js 파일 콜백함수 오류 수정
- 로그인 회원가입 submit 동작 시 input 값 value 및 error-message를 확인 후 /items 혹은 /signin 이동 결정. - 로그인 회원가입 submit 동작 시 input value를 확인하여 error-message 출력 여부 결정 - passwordcheckIcon 미작동 에러 수정 - 클래스 선언 및 경로 정리 - 버튼 focus 이벤트 삭제 (추후 재추가 예정)
- 심화 미션에 맞춰서 수정 title -> 판다 마켓 description -> 일상의 모든 물건을 거래해보세요
This reverts commit 7ee3f91.
- svg의 preserveAspectRatio="none" 으로 css로 크기 컨트롤 가능케 함
There was a problem hiding this comment.
창희님 3번째 스프린트 미션 고생하셨습니다.
아래는 전반적인 리뷰사항입니다
- 동일한 사항에 대해서는 한번만 코멘트를 답니다.
- 금주 제출사항에 대해서만 코멘트를 드립니다.
- 로그인 페이지에서 하단의 여백이 없는 것 같습니다. 디자인을 확인해보시고 추가해주시면 더 자연스러워 보일 것 같습니다.
- 인풋 상태에 따라 에러메시지를 보이게 작업해주셨는데, 에러메시지가 보이는 상태로 focus 시 에러메시지가 사라지는것이 약간 부자연스러워 보입니다. 인풋 value가 변경시 값을 재검사해 valid 확인을 하시는건 좋지만 focus시 되는것이 좋은지 생각해보시고 다른 홈페이지도 찾아보시면 좋겠네요.
- 자세히 쪼개서 commit 하신거 좋습니다.
커밋 내역은 merge하다가 꼬여서 어제 cherry-pick으로 한 번에 추가했습니다. 하루만에 몰아서 작업한 게 아님!: 쿨한 해결 좋습니다.제 코드가 너무 마음에 안들어욤.. 리팩토링 팁 없을까요 ㅠ: 어떤 부분이 마음에 안드시는지 알려주시면 첨언하기 좋을 것 같습니다~ 어떤 부분이 마음에 안드시는지, 어떻게 작업하고 싶으신지, 코드의 의도가 무엇인지 내 코드가 의도한 바를 잘 충족하는지 등을 고민해보세요.
| <link rel="stylesheet" href="/src/styles/base.css"> | ||
| <link rel="stylesheet" href="/src/styles/base-mobile.css"> | ||
| <link rel="stylesheet" href="/src/styles/base-tablet.css"> | ||
| <link rel="stylesheet" href="/src/styles/faq/layout.css"> | ||
| <link rel="stylesheet" href="/src/styles/faq/layout-tablet.css"> | ||
| <link rel="stylesheet" href="/src/styles/faq/layout-mobile.css"> |
There was a problem hiding this comment.
P3:
이렇게 화면 크기별로 나누게 되면 파일이 너무 많아질 가능성이 커집니다.
요즘은 pc, tablet, mobile 로 화면들이 딱 나눠지는 것이 아니라 이렇게 분리해 관리하기 더 어렵습니다.
또한 css 우선순위가 동일할 시 뒤에 선언된 css 문이 overriding 되는데 이렇게 나눠놓게 되면 우선순위 파악이 어렵고 한 페이지의 스타일링을 확인하기 위해서 파일 여러개를 확인해야 하므로
가능하면 한 파일로 관리하시는 것을 추천드립니다.
| </head> | ||
|
|
||
| <body style="display: flex; flex-direction: column; align-items: center;"> | ||
|
|
||
| <div data-include-component="/src/components/items/Items.html"></div> | ||
|
|
||
| <script src="/src/js/IncludeComponent.js"></script> | ||
| <script type="module" src="/src/js/ComponentLoader.js"></script> |
There was a problem hiding this comment.
P2:
script를 하단에 위치시키시는 이유가 무엇일까요?
만약 html이 다 로드된 후 js 실행을 시키기 위해서라면, 즉 지연 실행을 위해서라면 type이 module인 script는 자동으로 지연실행 되니 일반적인 위치인 header 쪽으로 옮기시는게 더 좋지 않을까합니다.
| <link rel="stylesheet" href="/src/styles/signin/layout-mobile.css"> | ||
| </head> | ||
|
|
||
| <body style="display: flex; flex-direction: column; align-items: center;"> |
There was a problem hiding this comment.
P3:
body 부분에 inline으로 스타일링 해야 하는 이유가 있을까요? 아니라면 layout.css와 같은 css 파일안에 작성하시는게 더 좋을 것 같습니다.
| @@ -0,0 +1,114 @@ | |||
| @media (min-width:375px) and (max-width:767px) { | |||
There was a problem hiding this comment.
P2:
이렇게 미디어쿼리 조건문을 작성하시면 375px 미만의 경우 base.css 작성된 스타일이 적용됩니다.
미디어 쿼리 조건문을 변경해주시거나 최소 사이즈 미만으로 줄어들 수 없게 상위 태그의 최소 너비를 제한해주시는게 좋습니다.
body {
min-width: 375px;
}
There was a problem hiding this comment.
P2:
pc, tablet, mobile 내에 중복되는 코드가 많은 것 같습니다.
이렇게 되면 디자인이 변경될 시 많은 코드를 변경해야 합니다.
가능하면 PC 작업을 하고, tablet css 작업 시 덮어써야하는것만 작성해주시는 것이 더 유지보수 하기 좋습니다.
There was a problem hiding this comment.
P3:
state-mobile, state-tablet, state.css 모두 동일한 내용인데 나눠서 관리해야 할 이유가 있는지 고민해보시면 좋겠습니다.
| passwordInput.addEventListener('focusin', () => hidePasswordErrorMessage(passwordErrorMessage)); | ||
| passwordIcon.addEventListener('click', () => togglePasswordIcon(passwordInput, passwordIcon)); | ||
| loginForm.addEventListener('submit', (event) => { | ||
| let bubblingFlag = routeToItems(emailInput, passwordInput, emailErrorMessage, passwordErrorMessage) |
There was a problem hiding this comment.
P2:
| let bubblingFlag = routeToItems(emailInput, passwordInput, emailErrorMessage, passwordErrorMessage) | |
| const bubblingFlag = routeToItems(emailInput, passwordInput, emailErrorMessage, passwordErrorMessage) |
There was a problem hiding this comment.
P3:
해당 함수를 보면 boolean값을 반환하는데 변수명으로는 이를 알 수가 없습니다.
또한 bubblineFlag라는 이름이 어떤 값인지 잘 설명하지 못하는것 같습니다.
isShowError, isValid같은 더 직관적이고 쉬운 변수명으로 작성하시면 좋을 것 같습니다.
| loginForm.addEventListener('submit', (event) => { | ||
| let bubblingFlag = routeToItems(emailInput, passwordInput, emailErrorMessage, passwordErrorMessage) | ||
| if(bubblingFlag) { | ||
| event.preventDefault(); |
There was a problem hiding this comment.
P2:
조건과 상관없이 중복되는 친구는 상단에 작성해주세요.
| * @param {span} passwordIcon - 비밀번호 아이콘 | ||
| */ | ||
| export const togglePasswordIcon = (passwordInput, passwordIcon) => { | ||
| const type = passwordInput.type == 'password' ? 'text' : 'password'; |
There was a problem hiding this comment.
P2:
| const type = passwordInput.type == 'password' ? 'text' : 'password'; | |
| const isEncrypt= passwordInput.type == 'password' ? 'text' : 'password'; |
| if (emailInput.value == '') { | ||
| emailErrorMessage.textContent = '이메일을 입력해주세요.'; | ||
| emailErrorMessage.style.display = 'block'; | ||
| } else if (!emailInput.value.includes('@')) { |
There was a problem hiding this comment.
P2:
early return 문을 활용하셔도 동일동작할 것 같습니다.
| if (emailInput.value == '') { | |
| emailErrorMessage.textContent = '이메일을 입력해주세요.'; | |
| emailErrorMessage.style.display = 'block'; | |
| } else if (!emailInput.value.includes('@')) { | |
| if (!emailInput.value.trim()) { // 가능하면 완전 일치 연산자를 써주시고 이렇게도 작성 가능합니다. | |
| emailErrorMessage.textContent = '이메일을 입력해주세요.'; | |
| emailErrorMessage.style.display = 'block'; | |
| return; | |
| } | |
| if (!emailInput.value.includes('@')) { |
기본
-------------------- 스프린트 3 미션 --------------------
PC: 1200px 이상
Tablet: 768px 이상 ~ 1199px 이하
Mobile: 375px 이상 ~ 767px 이하
375px 미만 사이즈의 디자인은 고려하지 않습니다
-- 회원가입 / 로그인 페이지
-------------------- 스프린트 4 미션 --------------------
-- 로그인
-- 회원가입
심화
-------------------- 스프린트 3 미션 --------------------
-------------------- 스프린트 4 미션 --------------------
주요 변경사항
=> 스프린트 2 미션 때 이런 기능도 구현해야하지 않을까? 해서 구현했더니 스프린트 4 미션의 내용이었습니다. 내용을 보니 이미 70% 이상 구현되어 있어서 그냥 마저 끝내자는 느낌으로 스프린트 4 미션까지 진행하였습니다.
(요구 사항으로 태블릿과 모바일은 padding 값이 고정되고 중앙의 영억이 줄어들게끔 설계되어있음)
스크린샷
배포 사이트
https://cheerful-salamander-19dcbf.netlify.app
멘토에게